src/services/platformApi.js (2)

59-84: Incomplete JSDoc block and duplicated query construction.

The fetchDatasets JSDoc block (lines 51-58) is left open, and a new JSDoc block for fetchAllEntities is started immediately after on line 59. This creates malformed documentation.

Additionally, the query parameter construction logic (lines 70-75) duplicates what's in fetchEntities (lines 30-36). Consider extracting a helper function to reduce duplication.

♻️ Proposed fix for JSDoc and extraction suggestion

   * `@throws` {Error} If the query fails or there is an error communicating with the Platform API
   */
+
+  /**
+   * Fetches all entities from the Platform API /entity.json endpoint, paginating through all results.
+   * Accepts the same params as fetchEntities (except limit/offset which are managed internally).
+   *
+   * `@param` {Object} params - Query parameters (same as fetchEntities)
+   * `@returns` {Promise<{formattedData: object[]}>} - All entities across all pages
+   */
-  /**
-   * Fetches all entities from the Platform API /entity.json endpoint, paginating through all results.
-   * Accepts the same params as fetchEntities (except limit/offset which are managed internally).
-   */
   fetchAllEntities: async (params) => {

Consider extracting a helper to build query params:

function buildEntityQueryParams(params, limit, offset) {
  const queryParams = new URLSearchParams()
  if (params.organisation_entity) queryParams.append('organisation_entity', params.organisation_entity)
  if (params.dataset) queryParams.append('dataset', params.dataset)
  if (params.prefix) queryParams.append('prefix', params.prefix)
  if (params.organisation) queryParams.append('organisation', params.organisation)
  if (limit) queryParams.append('limit', limit)
  if (offset) queryParams.append('offset', offset)
  if (params.quality) queryParams.append('quality', params.quality)
  return queryParams
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/platformApi.js` around lines 59 - 84, The JSDoc for
fetchDatasets is left unclosed and fetchAllEntities repeats query-building logic
from fetchEntities; close/fix the JSDoc block for fetchDatasets and extract the
duplicated URLSearchParams logic into a helper (e.g., buildEntityQueryParams)
then use that helper from both fetchEntities and fetchAllEntities to append
organisation_entity, dataset, prefix, organisation and limit/offset; ensure
buildEntityQueryParams accepts params, limit, offset (and optional quality) and
returns a URLSearchParams instance, then update fetchAllEntities to call the
helper instead of duplicating the query construction.

---

68-81: Consider adding a safety limit to prevent infinite loops.

The pagination logic is correct for normal API behaviour. However, if the Platform API has a bug returning links.next indefinitely, this could cause an infinite loop. Consider adding a maximum iteration safeguard.

🛡️ Optional safeguard

   fetchAllEntities: async (params) => {
     const pageSize = 100
     let offset = 0
     let allEntities = []
+    const maxPages = 1000 // Safety limit

-    while (true) {
+    for (let page = 0; page < maxPages; page++) {
       const queryParams = new URLSearchParams()
       // ... existing code ...
       if (!data?.links?.next || entities.length < pageSize) break
       offset += pageSize
     }
+
+    if (allEntities.length >= maxPages * pageSize) {
+      logger.warn({ message: 'fetchAllEntities reached maximum page limit', type: types.DataFetch, params })
+    }

     return { formattedData: allEntities }
   },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/platformApi.js` around lines 68 - 81, The while(true) pagination
loop in src/services/platformApi.js that calls queryPlatformAPI should include a
maximum iteration safeguard (e.g., MAX_PAGES) to avoid infinite loops if
data?.links?.next is always present; modify the loop that builds queryParams and
fetches entities so it increments a page counter each iteration and breaks (or
throws/logs a clear error) when the counter exceeds MAX_PAGES, ensuring existing
logic that checks data?.links?.next and entities.length < pageSize still runs;
update any function signature or local variables if needed (e.g., add a
configurable maxPages constant near pageSize/offset) and surface a clear error
message mentioning queryPlatformAPI and the endpoint when the max is reached.

src/views/organisations/overview.html (1)

181-209: Consider adding fallbacks for member/group names.

In the _planning-group-notice.html partial, there's a fallback to provision.organisation when provision.name is missing. However, lines 189 and 203 here use member.name and group.name directly without fallbacks. If the name property is missing, this could render empty link text.

♻️ Proposed fix to add fallbacks

-            <li><a class="govuk-link" href="/organisations/{{ member.organisation }}">{{ member.name }}</a></li>
+            <li><a class="govuk-link" href="/organisations/{{ member.organisation }}">{{ member.name or member.organisation }}</a></li>

-            <li><a class="govuk-link" href="/organisations/{{ group.organisation }}">{{ group.name }}</a></li>
+            <li><a class="govuk-link" href="/organisations/{{ group.organisation }}">{{ group.name or group.organisation }}</a></li>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/views/organisations/overview.html` around lines 181 - 209, The template
renders member.name and group.name directly which can produce empty link text;
update the two link texts in the planningGroupMembers and parentGroup loops to
use a fallback (e.g., show member.name falling back to member.organisation, and
group.name falling back to group.organisation) using your template engine’s
default/or operator or filter so the <a> text is never empty (these are the <li>
entries inside the planningGroupMembers loop and the parentGroup loop in the
planning group block—match the fallback style used in
_planning-group-notice.html).

src/middleware/common.middleware.js (1)

1193-1227: Consider caching the planning-group metadata.

This middleware now sits on several page paths, and each request performs a full local-planning-group scan, plus an organisation lookup when the current org is itself a planning group. A short TTL cache for the group list and org-name map would trim latency and make the new banners less dependent on the Platform API on every page view.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/middleware/common.middleware.js` around lines 1193 - 1227, The middleware
fetchLocalPlanningGroups currently queries platformApi.fetchAllEntities and
platformApi.fetchOrganisations on every request causing unnecessary latency; add
a short TTL in-memory cache for the fetched local-planning-group list and the
organisation name map so subsequent calls reuse cached data until expiry.
Implement a module-level cache object (e.g., cachedGroups, cachedOrgNameMap,
cachedAt, ttlMs) used by fetchLocalPlanningGroups: check cache validity before
calling platformApi.fetchAllEntities/fetchOrganisations, populate the caches and
cachedAt after a successful fetch, and fall back to existing behavior on cache
miss or error; ensure the try/catch/error path still sets req.parentGroup and
req.planningGroupMembers and does not throw. Reference symbols:
fetchLocalPlanningGroups, platformApi.fetchAllEntities,
platformApi.fetchOrganisations, req.parentGroup, req.planningGroupMembers.

src/routes/schemas.js (1)

120-126: Consider making planningGroupProvisions consistently an array (not optional).

Using v.optional(v.array(...)) means templates and callers must always handle undefined. If this field is part of the standard page contract, make it required as an array (empty when no data).

Suggested refactor

-const PlanningGroupProvisionsField = v.optional(v.array(v.strictObject({
+const PlanningGroupProvisionsField = v.array(v.strictObject({
   organisation: NonEmptyString,
   name: v.nullable(v.string()),
   dataset: NonEmptyString,
   project: v.nullable(v.string()),
   provision_reason: v.nullable(v.string())
-})))
+}))

Also applies to: 175-175, 202-202, 221-221, 242-242

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/routes/schemas.js` around lines 120 - 126, The schema currently defines
PlanningGroupProvisionsField as optional (v.optional(v.array(...))) which forces
callers to handle undefined; change it to a required array by replacing
v.optional(v.array(...)) with v.array(...) so the field is always an array
(empty when no items). Update the same pattern wherever planningGroupProvisions
is defined (the other occurrences referenced in the review) so all corresponding
schema constants use v.array(...) instead of v.optional(v.array(...)) to enforce
a consistent required-array contract; keep the inner item shape (organisation,
name, dataset, project, provision_reason) unchanged.

test/integration/planning_group.playwright.test.js (1)

27-27: Tighten the WireMock URL regex.

Line 27 uses entity.json with an unescaped dot, so it matches more than the literal path and can mask unrelated requests.

Suggested fix

-      urlPattern: '/entity.json.*prefix=local-planning-group.*'
+      urlPattern: '/entity\\.json.*prefix=local-planning-group.*'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/integration/planning_group.playwright.test.js` at line 27, The WireMock
urlPattern currently uses an unescaped dot in the string assigned to urlPattern
(the '/entity.json.*prefix=local-planning-group.*' pattern), which allows
unintended matches; update the urlPattern to escape the literal dot (use
'entity\\.json') and tighten the regex (e.g., anchor the query separator or
start of pattern) so it matches the exact path and expected query parameter
structure; modify the urlPattern assignment accordingly to use the corrected
regex.

src/routes/schemas.js (2)

118-118: Tighten OrgField.dataset to NonEmptyString.

Now that OrgFindPage is keyed by dataset, allowing dataset: '' inside OrgField is looser than both DatasetNameField.dataset and the new orgsByDataset record key. Reusing NonEmptyString here keeps the dataset contract consistent.

♻️ Proposed change

-const OrgField = v.strictObject({ name: NonEmptyString, organisation: NonEmptyString, statistical_geography: v.optional(v.string()), entity: v.optional(v.integer()), dataset: v.optional(v.string()) })
+const OrgField = v.strictObject({ name: NonEmptyString, organisation: NonEmptyString, statistical_geography: v.optional(v.string()), entity: v.optional(v.integer()), dataset: v.optional(NonEmptyString) })

Also applies to: 168-168

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/routes/schemas.js` at line 118, OrgField currently allows dataset:
v.optional(v.string()) which is too permissive; change the dataset schema in the
OrgField definition to use NonEmptyString (e.g., dataset:
v.optional(NonEmptyString)) so it matches DatasetNameField.dataset and the
orgsByDataset record key; update the OrgField declaration (and the other
occurrence at the second instance mentioned) to replace v.optional(v.string())
with v.optional(NonEmptyString).

---

155-164: Extract the repeated planning-group object schemas.

These additions make sense, but parentGroup is duplicated verbatim across both overview schemas, and planningGroupMembers is another inline strict object. Pulling them into shared constants would make future field changes less error-prone.

♻️ Proposed refactor

+const ParentGroupField = v.optional(v.nullable(v.array(v.strictObject({
+  entity: v.integer(),
+  name: NonEmptyString,
+  organisation: NonEmptyString
+}))))
+
+const PlanningGroupMembersField = v.optional(v.nullable(v.array(v.strictObject({
+  organisation: NonEmptyString,
+  name: NonEmptyString
+}))))
+
 export const OrgOverviewPage = v.strictObject({
   organisation: OrgField,
   datasets: v.object({
@@
   datasetsWithErrors: v.integer(),
   isODPMember: v.boolean(),
-  parentGroup: v.optional(v.nullable(v.array(v.strictObject({
-    entity: v.integer(),
-    name: NonEmptyString,
-    organisation: NonEmptyString
-  })))),
-  planningGroupMembers: v.optional(v.nullable(v.array(v.strictObject({
-    organisation: NonEmptyString,
-    name: NonEmptyString
-  }))))
+  parentGroup: ParentGroupField,
+  planningGroupMembers: PlanningGroupMembersField
 })
@@
   }),
   planningGroupProvisions: PlanningGroupProvisionsField,
-  parentGroup: v.optional(v.nullable(v.array(v.strictObject({
-    entity: v.integer(),
-    name: NonEmptyString,
-    organisation: NonEmptyString
-  })))),
+  parentGroup: ParentGroupField,
   notice: v.optional(DeadlineNoticeField)
 })

Also applies to: 203-207

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/routes/schemas.js` around lines 155 - 164, The parentGroup and
planningGroupMembers object schemas are duplicated inline; extract them into
shared schema constants (e.g., ParentGroupSchema and PlanningGroupMemberSchema)
defined once and reuse them where parentGroup and planningGroupMembers are
declared: ParentGroupSchema should be v.strictObject({ entity: v.integer(),
name: NonEmptyString, organisation: NonEmptyString }) and
PlanningGroupMemberSchema should be v.strictObject({ organisation:
NonEmptyString, name: NonEmptyString }); then replace the inline
v.array(v.strictObject(...)) usages with v.array(ParentGroupSchema) and
v.array(PlanningGroupMemberSchema) wrapped in the same
v.optional(v.nullable(...)) as before so both overview schemas reference the
single shared definitions.

src/utils/datasetSlugToReadableName.js (2)

21-24: The fire-and-forget pattern is intentional and sound, but consider adding observability.

The non-awaited call to retryUntilLoaded() allows the server to start immediately with graceful degradation. This aligns with the PR objective of continuous retry on boot failure.

However, there's currently no way to know whether the mapping has successfully loaded (e.g., for health checks or operational monitoring). Consider exposing a status indicator.

💡 Optional: Add a flag to indicate mapping load status

 let datasetSlugToReadableName = (slug) => slug
+let mappingLoaded = false

 const retryUntilLoaded = async () => {
   while (true) {
     try {
       const mapping = await getDatasetSlugNameMapping()
       datasetSlugToReadableName = makeDatasetSlugToReadableNameFilter(mapping)
+      mappingLoaded = true
       return
     } catch (error) {
       // ...
     }
   }
 }

+const isMappingLoaded = () => mappingLoaded

 export {
   datasetSlugToReadableName,
-  initDatasetSlugToReadableNameFilter
+  initDatasetSlugToReadableNameFilter,
+  isMappingLoaded
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/datasetSlugToReadableName.js` around lines 21 - 24, The
initDatasetSlugToReadableNameFilter currently calls retryUntilLoaded()
fire-and-forget, so add observable state: introduce a boolean flag (e.g.,
isDatasetMappingLoaded) initialized false and set it true inside
retryUntilLoaded (or its success callback), export a getter function (e.g.,
isMappingLoaded or getDatasetMappingStatus) alongside datasetSlugToReadableName,
and update retryUntilLoaded to update that flag on success and optionally expose
a promise (e.g., mappingLoadedPromise) for health checks to await; reference
initDatasetSlugToReadableNameFilter, retryUntilLoaded, and
datasetSlugToReadableName when making these changes.

---

14-16: Use the structured logger instead of console.error for consistency.

The codebase uses structured logging elsewhere (e.g., logger.error() in src/utils/datasetteQueries/fetchDatasetCollections.js). Using console.error here bypasses any structured logging configuration, log aggregation, or filtering set up for the application.

♻️ Suggested change

+import logger from '../utils/logger.js'
+import { types } from '../utils/logging.js'

 // ...

     } catch (error) {
-      console.error(`Failed to load dataset mapping, retrying in ${RETRY_INTERVAL_MS / 1000}s:`, error)
+      logger.error({
+        message: `Failed to load dataset mapping, retrying in ${RETRY_INTERVAL_MS / 1000}s`,
+        error,
+        type: types.DataFetch
+      })
       await new Promise(resolve => setTimeout(resolve, RETRY_INTERVAL_MS))
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/datasetSlugToReadableName.js` around lines 14 - 16, Replace the
console.error in the catch block of datasetSlugToReadableName.js with the
project's structured logger: call logger.error(...) passing a clear message
(e.g., "Failed to load dataset mapping, retrying") plus the error object and
include RETRY_INTERVAL_MS for context; if logger isn't imported in that module,
add the same logger import used elsewhere (matching other files like
fetchDatasetCollections.js) so the structured logger is available, then keep the
existing await new Promise(...) retry logic unchanged.

src/utils/datasetSlugToReadableName.js (1)

9-20: Retry loop implementation is sound, but consider adding a maximum retry count or backoff.

The infinite retry with fixed 30-second intervals will eventually succeed, but consider:

1. Adding exponential backoff to reduce load on a struggling upstream service
2. Logging the attempt count for observability
3. Optionally capping retries and failing loudly if the service is fundamentally unavailable

♻️ Optional: add attempt counter for observability

 const retryUntilLoaded = async () => {
+  let attempt = 0
   while (true) {
+    attempt++
     try {
       const mapping = await getDatasetSlugNameMapping()
       datasetSlugToReadableName = makeDatasetSlugToReadableNameFilter(mapping)
+      logger.info(`Dataset mapping loaded successfully after ${attempt} attempt(s)`)
       return
     } catch (error) {
-      logger.warn(`Failed to load dataset mapping, retrying in ${RETRY_INTERVAL_MS / 1000}s:`, error)
+      logger.warn(`Failed to load dataset mapping (attempt ${attempt}), retrying in ${RETRY_INTERVAL_MS / 1000}s:`, error)
       await new Promise(resolve => setTimeout(resolve, RETRY_INTERVAL_MS))
     }
   }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/datasetSlugToReadableName.js` around lines 9 - 20, The
retryUntilLoaded loop should be made bounded and backoff-aware: add an attempt
counter and a maxAttempts constant, log the attempt number in logger.warn,
implement exponential backoff based on RETRY_INTERVAL_MS (e.g., wait =
RETRY_INTERVAL_MS * 2**(attempt-1) with a cap) before the next
getDatasetSlugNameMapping try, and after exceeding maxAttempts either throw the
error or call a failure handler so the process fails loudly; update references
inside retryUntilLoaded and keep makeDatasetSlugToReadableNameFilter and
getDatasetSlugNameMapping usage unchanged.

test/testContainers/localstack.js (1)

16-20: Consider adding a comment to explain the testcontainers reuse pattern.

The stop() method's logic—creating and "starting" a container only to immediately stop it—is intentional for testcontainers with .withReuse(true) (which returns the existing running container), but it reads as counterintuitive. A brief comment would help future maintainers understand this behaviour.

📝 Suggested clarifying comment

 async stop () {
   console.log('Stopping LocalstackContainer')
+  // With reuse enabled, .start() returns the already-running container rather than creating a new one.
   this.container = await new LocalstackContainer(this.image).withReuse(true).start()
   await this.container.stop()
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/testContainers/localstack.js` around lines 16 - 20, The stop()
implementation is intentionally creating a new LocalstackContainer and calling
.withReuse(true).start() to obtain the already-running reusable container before
calling stop(), but this is not obvious; add a brief inline comment inside the
stop() method (next to the this.container = await new
LocalstackContainer(this.image).withReuse(true).start() line) explaining that
withReuse(true) returns the existing running container so we start to attach to
it and then call this.container.stop() to stop the reused instance, clarifying
the counterintuitive flow for future maintainers (reference: stop(),
LocalstackContainer, withReuse(true)).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes